-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable launch test discovery in pytest #312
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
1af785b
to
94922bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some minimal comments.
I surfed in pytest plugin documentation and I don't fully understand the patch. But I don't have much more to add if it's working.
def find_launch_test_entrypoint(path): | ||
try: | ||
return getattr(path.pyimport(), 'generate_test_description', None) | ||
except SyntaxError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that's the only possible exception, I would catch everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A SyntaxError
is the only error I'm expecting. I'd rather not catch (and hide) unexpected errors e.g. some exception as a result of code being executed at the module level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is old code, but just as reference:
https://github.com/ros2/launch_ros/blob/bf3d800a53ea53d9465da9298c104998a7461dcf/ros2launch/ros2launch/command/launch.py#L125-L141
Couldn't value error happen here too?
Extra comment, instead of Also, aren't files with |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
…unch_testing plugin. Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Right now, any module in the search path that has a
Nope, this hook collects them appropriately once. |
I had to fix other failing tests, due to examples not being installed and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with green CI
def __iter__(self): | ||
return iter(self.__calls) | ||
yield partial, partial_args | ||
setattr(_wrapped, '__parametrized__', True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't be:
_wrapped.__parametrized__ = True
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it can be. Done in 0705777.
""" | ||
A module providing exit code assertions. | ||
|
||
PYTEST_DONT_REWRITE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, add a comment/link explaining why this is needed (e.g.: https://docs.pytest.org/en/latest/assert.html#assertion-introspection-details).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, done in c7e6f80.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Alright, all green! Going in. |
Closes #237. This pull request allows for launch tests discovery and execution via
pytest
. It also sets up the scene for downstream packages i.e.launch_testing_ros
to customize this.Note this is not a tight integration between
launch_testing
andpytest
but simply a mechanism for the latter to be able to execute launch tests.Depends on #308.